SUM aggregation enhancement on operations with literal #3971

qianheng-aws · 2025-08-04T09:23:56Z

Description

SUM aggregation enhancement on operations with literal

Related Issues

Resolves #3967

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws · 2025-08-04T15:06:39Z

integ-test/src/test/java/org/opensearch/sql/calcite/clickbench/PPLClickBenchIT.java

  /**
-   * Ignore queries that are not supported by Calcite. Ignore q30 because of too much script push
-   * down, which will cause ResourceMonitor restriction.
+   * Ignore queries that are not supported by Calcite. TODO: need to investigate on why q30 trigger


Calcite will produce lots of recyclable memory garbage during its planning and implementation process. For q30, it produces around 70 mb in optimizing + 30 mb in implementation on my local test, which can all be GC.

So I think, for Calcite, the ResourceMonitor's memory health check is unpredictable and less reasonable.

ResourceMonitor protect physical operator, how it related to Calcite optimizing/implementation logica?

sql/opensearch/src/main/java/org/opensearch/sql/opensearch/monitor/OpenSearchMemoryHealthy.java

Lines 53 to 58 in 7892b90

static class MemoryUsage {

public long usage() {

final long freeMemory = Runtime.getRuntime().freeMemory();

final long totalMemory = Runtime.getRuntime().totalMemory();

return totalMemory - freeMemory;

}

ResourceMonitor get the memory usage by using total memory - free memory. While the whole process of a query(even other queries previous to the current one) will accumulate the memory usage until GC.

I've submitted a PR #3983 to fix the issue

penghuo · 2025-08-04T23:41:12Z

...earch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java


+  /** See source in {@link org.apache.calcite.rel.core.Aggregate::computeSelfCost} */
+  private static float getAggMultiplier(PushDownAction action) {
+    List<AggregateCall> aggCalls = ((Aggregate) action.digest).getAggCallList();


Can we add a comments before and after code from calcite

// START CALCITE
...
your code
// END CALCITE

penghuo · 2025-08-04T23:42:06Z

...earch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java

+    // For script aggregation, we need to multiply the multiplier by 2.2 to make up the cost. As we
+    // prefer to have non-script agg push down after optimized by {@link PPLAggregateConvertRule}
+    if (((AggPushDownAction) action.action).isScriptPushed) {
+      multiplier *= 2.2f;


2.2f magic number, Can i be changed in future?

Usually it doesn't need any change unless we met some edge case(like the final plan doesn't satisfy our expectation) where I need refactor it.

Similar to the factor we add in filter script push down:

sql/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java

Line 128 in 7892b90

* 1.1;

penghuo · 2025-08-04T23:56:25Z

integ-test/src/test/java/org/opensearch/sql/calcite/clickbench/PPLClickBenchIT.java

  /**
-   * Ignore queries that are not supported by Calcite. Ignore q30 because of too much script push
-   * down, which will cause ResourceMonitor restriction.
+   * Ignore queries that are not supported by Calcite. TODO: need to investigate on why q30 trigger


ResourceMonitor protect physical operator, how it related to Calcite optimizing/implementation logica?

penghuo · 2025-08-05T00:09:34Z

core/src/main/java/org/opensearch/sql/calcite/plan/PPLAggregateConvertRule.java

+import org.immutables.value.Value;
+
+/**
+ * Planner rule that converts specific aggCall to a more efficient expressions, which includes: -


penghuo · 2025-08-05T00:14:08Z

core/src/main/java/org/opensearch/sql/calcite/plan/PPLAggregateConvertRule.java

+ * NUMBER, Don't support this because of integer division - MAX/MIN(FIELD [+|-|*|+|/] NUMBER) ->
+ * MAX/MIN(FIELD) [+|-|*|+|/] NUMBER


format java doc.

penghuo · 2025-08-05T00:15:15Z

core/src/main/java/org/opensearch/sql/calcite/plan/PPLAggregateConvertRule.java

+/**
+ * Planner rule that converts specific aggCall to a more efficient expressions, which includes: -
+ * SUM(FIELD + NUMBER) -> SUM(FIELD) + NUMBER * COUNT() - SUM(FIELD - NUMBER) -> SUM(FIELD) - NUMBER
+ * * COUNT() - SUM(FIELD * NUMBER) -> SUM(FIELD) * NUMBER - SUM(FIELD / NUMBER) -> SUM(FIELD) /


SUM(FILED / NUMBER) -> SUM(FIELD) / NUMBER, does it work for float/double?

No, they also have precision issue, e.g:

assuming SUM(a / 0.2) and the table has a = 0.1 for its all 3 rows. The execution could be present:

0.1 / 0.2 * 3 = 1.5

0.1 * 3 / 0.2 = 1.5000000000000002

Signed-off-by: Heng Qian <qianheng@amazon.com>

# Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

Signed-off-by: Heng Qian <qianheng@amazon.com>

# Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

qianheng-aws · 2025-08-20T09:07:59Z

@penghuo Please take another look.

opensearch-trigger-bot · 2025-08-21T20:23:51Z

The backport to 2.19-dev failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-3971-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 892355190687033b0f29bae624182c1ba204bf96
# Push it to GitHub
git push --set-upstream origin backport/backport-3971-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-dev

Then, create a pull request where the base branch is 2.19-dev and the compare/head branch is backport/backport-3971-to-2.19-dev.

…oject#3971) * SUM aggregation enhancement on operations with literal Signed-off-by: Heng Qian <qianheng@amazon.com> * Fix CI Signed-off-by: Heng Qian <qianheng@amazon.com> * Keep ignoring q30 for Calcite Signed-off-by: Heng Qian <qianheng@amazon.com> * Add UT for PPLAggregateConvertRule Signed-off-by: Heng Qian <qianheng@amazon.com> * Add UT for PPLAggregateConvertRule Signed-off-by: Heng Qian <qianheng@amazon.com> * Spotless check Signed-off-by: Heng Qian <qianheng@amazon.com> --------- Signed-off-by: Heng Qian <qianheng@amazon.com> Co-authored-by: Mitchell Gale <mitchell.gale@improving.com> (cherry picked from commit 8923551)

…teral (#3971) (#4104) * SUM aggregation enhancement on operations with literal (#3971) * SUM aggregation enhancement on operations with literal Signed-off-by: Heng Qian <qianheng@amazon.com> * Fix CI Signed-off-by: Heng Qian <qianheng@amazon.com> * Keep ignoring q30 for Calcite Signed-off-by: Heng Qian <qianheng@amazon.com> * Add UT for PPLAggregateConvertRule Signed-off-by: Heng Qian <qianheng@amazon.com> * Add UT for PPLAggregateConvertRule Signed-off-by: Heng Qian <qianheng@amazon.com> * Spotless check Signed-off-by: Heng Qian <qianheng@amazon.com> --------- Signed-off-by: Heng Qian <qianheng@amazon.com> Co-authored-by: Mitchell Gale <mitchell.gale@improving.com> (cherry picked from commit 8923551) * Fix Compiling Signed-off-by: Heng Qian <qianheng@amazon.com> * Fix Compiling Signed-off-by: Heng Qian <qianheng@amazon.com> * Fix Compiling Signed-off-by: Heng Qian <qianheng@amazon.com> --------- Signed-off-by: Heng Qian <qianheng@amazon.com> Co-authored-by: Mitchell Gale <mitchell.gale@improving.com> Co-authored-by: MitchellGale-BitQuill <104795536+mitchellgale-bitquill@users.noreply.github.com>

SUM aggregation enhancement on operations with literal

1e08eb0

Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws requested review from GumpacG, LantaoJin, MaxKsyunz, Swiddis, YANG-DB, Yury-Fridlyand, acarbonetto, anirudha, dai-chen, derek-ho, forestmvey, joshuali925, kavithacm, mengweieric, noCharger, penghuo, ps48, seankao-az and ykmr1224 as code owners August 4, 2025 09:23

qianheng-aws added enhancement New feature or request PPL Piped processing language calcite calcite migration releated backport 2.19-dev labels Aug 4, 2025

qianheng-aws added 2 commits August 4, 2025 18:22

Fix CI

2a9b070

Signed-off-by: Heng Qian <qianheng@amazon.com>

Keep ignoring q30 for Calcite

8146026

Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws commented Aug 4, 2025

View reviewed changes

penghuo reviewed Aug 5, 2025

View reviewed changes

qianheng-aws mentioned this pull request Aug 5, 2025

[BUG] ResourceMonitor Incorrectly Triggers Unhealthy Status Despite Recyclable Memory #3981

Closed

Add UT for PPLAggregateConvertRule

54c2717

Signed-off-by: Heng Qian <qianheng@amazon.com>

Add UT for PPLAggregateConvertRule

a51a9a8

Signed-off-by: Heng Qian <qianheng@amazon.com>

LantaoJin mentioned this pull request Aug 5, 2025

ResourceMonitor only checks the memory health by calculating the memory usage after GC of old gen in v3 #3983

Merged

7 tasks

qianheng-aws and others added 4 commits August 15, 2025 11:18

Merge remote-tracking branch 'refs/remotes/origin/main' into SumEnhance

b8275c7

# Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

Spotless check

cfa7574

Signed-off-by: Heng Qian <qianheng@amazon.com>

Merge remote-tracking branch 'refs/remotes/origin/main' into SumEnhance

efacb9c

# Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

Merge remote-tracking branch 'refs/remotes/origin/main' into SumEnhance

a4bad8a

LantaoJin approved these changes Aug 20, 2025

View reviewed changes

penghuo approved these changes Aug 21, 2025

View reviewed changes

penghuo merged commit 8923551 into opensearch-project:main Aug 21, 2025
23 checks passed

opensearch-trigger-bot bot added the backport-failed label Aug 21, 2025

LantaoJin added the backport-manually Filed a PR to backport manually. label Sep 9, 2025

tkykenmt mentioned this pull request Jan 11, 2026

[feature] SQL/PPL Aggregation Pushdown tkykenmt/opensearch-feature-explorer#1327

Closed

3 tasks

	static class MemoryUsage {
	public long usage() {
	final long freeMemory = Runtime.getRuntime().freeMemory();
	final long totalMemory = Runtime.getRuntime().totalMemory();
	return totalMemory - freeMemory;
	}

		* NUMBER, Don't support this because of integer division - MAX/MIN(FIELD [+\|-\|*\|+\|/] NUMBER) ->
		* MAX/MIN(FIELD) [+\|-\|*\|+\|/] NUMBER

SUM aggregation enhancement on operations with literal #3971

SUM aggregation enhancement on operations with literal #3971

Uh oh!

Conversation

qianheng-aws commented Aug 4, 2025

Description

Related Issues

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianheng-aws commented Aug 20, 2025

Uh oh!

Uh oh!

opensearch-trigger-bot bot commented Aug 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qianheng-aws Aug 5, 2025 •

edited

Loading